cli keeps calling non paginated endpoint even when interactive flag is on #847
Conversation
Codecov ReportBase: 87.05% // Head: 87.06% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #847 +/- ##
=======================================
Coverage 87.05% 87.06%
=======================================
Files 100 100
Lines 8652 8674 +22
=======================================
+ Hits 7532 7552 +20
- Misses 653 654 +1
- Partials 467 468 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
ajayy004
requested review from
andriisoldatenko,
aliotta,
neel-astro,
AmFlint,
kushalmalani,
sunkickr and
jemishp
as code owners
| var err error | ||
|
|
||
| if interactive { | ||
| workspacePageSize := 2 |
There was a problem hiding this comment.
why not use the PageSize value from config.CFG.PageSize or else defaultPageSize?
Reference: https://github.com/astronomer/astro-cli/blob/main/software/auth/auth.go#L135
There was a problem hiding this comment.
Since we need to identify if the number of workspaces returned 1 or more, calling workspace pagination with page size 2 if the interactive flag is set to true, pulling 100 records does not make sense as we just need to make a decision of 1 or more.
There was a problem hiding this comment.
That's not entirely true, because the list of workspaces returned from this method is used to switch to last_used_workspace, and by just pulling two workspaces, that functionality will almost always be broken in case interactive is set to true.
So from a UX point of view, if interactive is set to true, then whenever a user tries to log in, they would be asked to select a particular workspace every single time, and not automatically set the last used workspace from the previous login as the current workspace.
We should document this in our release note if we are going with this approach, and also add comments in the code around the logic of choosing the page size as 2 for future reference.
Might want to check once on this functional change with @cmarteepants if not already done.
There was a problem hiding this comment.
The trade-off (selecting the workspace every single time a user logs in) is too expensive from a UX standpoint. I'm not following how the change helps solve the initial problem. Is there another approach we can use to solve the problem, while retaining the login UX?
There was a problem hiding this comment.
As discussed, I'll change the implementation of switchToLastUsedWorkspace to use getWorkspace API to identify if the user still has access/available last_used_workspace workspace. That way will have the same user experience.
…ad of looping through workspaces * added get workspace query without rolebinding to validate workspace id
… roleBindings since its not used to validate workspaceId
…dentify user has access to more than 1 workspace access
…s on (#847) * call workspace pagination query when interactive flag set to true * fixed lint issue * * changed switchToLastUsedWorkspace function to query workspace instead of looping through workspaces * added get workspace query without rolebinding to validate workspace id * replaced getWorkspace query with ValidateWorkspaceId to avoid pulling roleBindings since its not used to validate workspaceId * fixed lint issue * added test coverage for ValidateWorkspaceID * added test coverage for switchToLastUsedWorkspace * added comment, why we need call with certain workspace page size to identify user has access to more than 1 workspace access
…s on (#847) * call workspace pagination query when interactive flag set to true * fixed lint issue * * changed switchToLastUsedWorkspace function to query workspace instead of looping through workspaces * added get workspace query without rolebinding to validate workspace id * replaced getWorkspace query with ValidateWorkspaceId to avoid pulling roleBindings since its not used to validate workspaceId * fixed lint issue * added test coverage for ValidateWorkspaceID * added test coverage for switchToLastUsedWorkspace * added comment, why we need call with certain workspace page size to identify user has access to more than 1 workspace access
…s on (#847) * call workspace pagination query when interactive flag set to true * fixed lint issue * * changed switchToLastUsedWorkspace function to query workspace instead of looping through workspaces * added get workspace query without rolebinding to validate workspace id * replaced getWorkspace query with ValidateWorkspaceId to avoid pulling roleBindings since its not used to validate workspaceId * fixed lint issue * added test coverage for ValidateWorkspaceID * added test coverage for switchToLastUsedWorkspace * added comment, why we need call with certain workspace page size to identify user has access to more than 1 workspace access
Description
Issue: CLI makes a workspace list query to identify if there is 1 or more workspace
Fix: If the interactive flag is set to true, CLI will call the paginated workspace query with page size 2 to make the same decision to auto-select the first workspace if it returns 1 record if it returns more than 1. The user will be prompted to select a workspace
Enhancement: Added new query to validate
workspaceId, which does not pullroleBindingssince it's not required to validateworkspaceId.🎟 Issue(s)
Related https://github.com/astronomer/issues/issues/5166
🧪 Functional Testing
📸 Screenshots
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft